-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add rmdir support #2985
Add rmdir support #2985
Conversation
f3a340c
to
1e32dc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks fine, think there's room for improvement around the helper function you added though.
Some of our logs are in the Thing -> {foo}
format because we were using a different logging crate (or something like that) in the past. Now we mostly just stick the tracing::instrument
macro on each function, and the old style is still there in some places due to us changing it only when we touch the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! My only suggestion other than what @meowjesty already suggested is changing the changelog file name to 2221.added.md
since new features are being added rather than existing ones being changed, but up to you (I know the changlog for the mkdir support PR was fixed
) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the PR!
Approving it since I only have 2 nits.
Hey @facundopoblete, sorry for not merging. We forgot that you can't do it yourself 😅 |
@Razz4780 No problem! Done |
Add support to forward the
rmdir
command to the agent